I think @maxomatic458 would be the ideal reviewer here, based on recent work in the completion logic and the discussion we had around the pitfalls with this highlighting.

looks good so far.

I think instead of defaulting to this
https://github.com/ysthakur/reedline/blob/44e18841807ef914d302f57fc9cb9b2c151c099b/src/menu/ide_menu.rs#L516-L523

it might be better to just ignore the styling (so basically fully leave it up to the completer).

Then we could get rid of this trait function here (since we have the match_indices now):
https://github.com/ysthakur/reedline/blob/44e18841807ef914d302f57fc9cb9b2c151c099b/src/completion/base.rs#L39-L51

If you are ok with that i can make a PR for that.

@maxomatic458 Interesting idea, I was going to say highlighting shouldn't be the completer's responsibility, but if it simplifies things, that'd be great. It's definitely worth a PR. @sholderbach Thoughts?

I was going to say highlighting shouldn't be the completer's responsibility, but if it simplifies things, that'd be great.

How would that interact with other highlighting functionality in reedline? Eg. it takes care of highlighting the selected item, if the completer dealt with highlighting individual characters how would that mix?

it might be better to just ignore the styling (so basically fully leave it up to the completer).

Do you mean decoupling value from how it's displaying, eg. adding something like display_value to Suggestion, while value is still what gets put on the command line on selection?

Slightly tangential, but there's plenty of places that initialize Suggestion structs and have to deal with setting lots of fields to None (though mostly in nushell proper it seems) - maybe it's time to implement use Default?

Do you mean decoupling value from how it's displaying, eg. adding something like display_value to Suggestion, while value is still what gets put on the command line on selection?

I think this might actually be better solution. So instead of adding more stuff to the Suggestion we just have one display_value

that can be modified by a seperate styling layer (that can be attached to a completer).
This would also make styling way more customizable.

But this might require a good amount of breaking changes, so maybe thats something for a future PR?

How would that interact with other highlighting functionality in reedline? Eg. it takes care of highlighting the selected item, if the completer dealt with highlighting individual characters how would that mix?

Ah, you're right @qfel, highlighting in the completer wouldn't work.

I think this might actually be better solution. So instead of adding more stuff to the Suggestion we just have one display_value

@maxomatic458 As @qfel pointed out, we'd need two display values, one in case the suggestion is selected and one in case it isn't, right? That feels like too much to me.

that can be modified by a seperate styling layer (that can be attached to a completer).
This would also make styling way more customizable.

@maxomatic458 This sounds interesting, could you explain more? (having a hard time figuring out what a separate styling layer would look like)

@ysthakur

maybe this could be implemented as a trait
something like

pub trait SuggestionStyler {
    fn style_selected(&self, suggestion: Suggestion, ...) -> String {
        ...
    }
    
    fn style_unselected(&self, suggestion: Suggestion, ...) -> String {
        ...
    }
}

then we could pass a SuggestionStyler struct down to the menu and inside the create_string function of the menu we can style the suggestions

the functions of the SuggestionStyler trait could have paremeters like typed text, cursor pos

so we would move the styling out of the menu and out of the completer and we can implement it using this trait

Edit:
if we would add a display_value field to the suggestion we could just mutate it inside the trait functions,
so we would just need one field, and that would always be the string for whats currently being drawn (so not dependant on wether its selected or not)

@maxomatic458 Sorry, still not getting it. Do you mean that the style field would be removed from Suggestion, the match_indices field from this PR would still be added, and then a SuggestionStyler would perform styling? And this styler could output by mutating a display_value field on the suggestion?

@ysthakur
that approach would not need any more fields on the Suggestion itself
all styling would be handled inside this trait

that would potentially allow for more complex styling, without having to make any changes to the menu

so something like this here

enum MatchMode {
    Fuzzy,
    Prefix,
}

struct UnderlineStyler {
    color: ...,
    mode: MatchMode
}

impl SuggestionStyler for UnderlineStyler {
    fn style_selected(&mut self, suggestion: Suggestion, cursor_pos: Option<(u16, u16)>, text_typed_by_user: String, terminal_dims: Option<(u16, u16)>, ...) -> Suggestion {
        // Do the styling here
        ...
        return suggestion
    }
    
    fn style_unselected(...) -> Suggestion {
         ...
    }
}

then we could add this Styler as a field to the menus

and then call the trait functions in menu when we are generating the strings for the suggestions

Thats just an idea how it could be implemented if we want to make the suggestion-styling that extensible.
But i also understand that there probably isnt really a need to make it that extensible atm. But that would just avoid having to add more fields to the Suggestions struct in the future.

Im also ok with just adding the match_indices instead

IIUC that would either mean we keep match_indices so that Styler can use the information to style part of the suggestion - in which case that seems fully orthogonal to this PR and better discuss outside (so we can make the smaller incremental change here).

Or it would mean inside the styler we have to recompute the matching indices, which sounds a bit unpleasant. Unless we allow to attach arbitrary data with suggestions - then completers could generate suggestions with completer-specific hints, and provide stylers that consume those hints to producer stylized output. Though that feels like a fairly big change?

Or it would mean inside the styler we have to recompute the matching indices, which sounds a bit unpleasant

That was my idea (if you are concerned about the recomputing, i believe in both cases we would need to recompute the indices whenever the user types something). We would attach a Styler to a completer, and that would handle the styling of the suggestions.

so the Suggestions struct would not have to be modified again, when we want to introduce a new way of styling suggestions.

@ysthakur
that approach would not need any more fields on the Suggestion itself
all styling would be handled inside this trait

@maxomatic458 Oh, I see now. I like the idea of avoiding adding fields to Suggestion to control styling, but as qfel said, recomputing the matching indices sounds unpleasant. Also, both the completer and the styler will need to be aware of how matching works.

That said, I agree with you that performance might not be a huge problem. Only a few suggestions are displayed on the screen at any point, even if there are hundreds of suggestions in total. If we only recompute the match indices for each suggestion when it shows up on the screen, the user may not see a noticeable delay.

I'm not an expert here, though. Anyone have any idea what impact this would have on performance?

so the Suggestions struct would not have to be modified again, when we want to introduce a new way of styling suggestions.

@maxomatic458 Did you have any possible future ways of styling suggestions in mind? Do you mean we might want to highlight suggestions in three different colors (rather than the current two)?

@ysthakur

Also, both the completer and the styler will need to be aware of how matching works.

Ah now i see, yea in that case we would need to do the same calculation twice.

Did you have any possible future ways of styling suggestions in mind? Do you mean we might want to highlight suggestions in three different colors (rather than the current two)?

yes multicolored stuff would be possible.
Nushell currently has different colored file suggestions per filetype (i think you need to enable it in the config)

So if we provide file system information to the styler stuff like this would be possible.
We could even mutate the text of the suggestion (without changing the completion value if we add a display_value field)
so maybe we could add info like file size, or edit date or stuff like that to the suggestion.

like:

File 1 (1 GB) yesterday
File 2 (2 GB) 10 days ago

and because we can store data in the Styler struct, we could even cache stuff like this, so we dont need to fetch it every keystroke

I agree with you that performance might not be a huge problem.

That aside, potentially having to run the same matching code in two different places in the Completer is an unpleasant API. Not a huge issue I suppose, but makes it a little harder to get things going (and might have some weird implications for non-deterministic/pure matching, maybe if somebody decided to do some server-side completions).

terminal_dims: Option<(u16, u16)>

I noticed one of the examples above had this - what would it be used for?

Hm let me expand a bit on the styler idea that I think could work well (if we really want to go there):

1. We split Suggestion into two separate types, so to not mix presentation with completion (plus for some fields it would make no sense to change them in Styler, so why have it push them trough):

pub struct Suggestion<T> {
    /// String replacement that will be introduced to the the buffer
    pub value: String,
    /// Replacement span in the buffer
    pub span: Span,
    /// Whether to append a space after selecting this suggestion.
    /// This helps to avoid that a completer repeats the complete suggestion.
    pub append_whitespace: bool,
    /// Completer-associated context, passed back to Styler.
    pub context: T,
}

pub struct DisplaySuggestion {
    /// The value to display, often the same as coming from Suggestion but will have ANSI escapes.
    pub value: String,
    /// Optional description for the replacement, can contain ANSI escapes.
    pub description: Option<String>,
    /// Optional vector of strings in the suggestion. These can be used to
    /// represent examples coming from a suggestion. Can contain ANSI escapes.
    pub extra: Option<Vec<String>>,
}

2. We introduce a Styler trait, to convert Suggestion to DisplaySuggestion:

// Probably needs more thought about the naming.
pub trait Styler<T> {
  fn display(&self, suggestion: Suggestion<T>, is_selected: bool, some_type_of_user_config: _, ...) -> DisplaySuggestion;
  // Also batch_display with some default implementation. May be useful for some completers to parallelize.
}

3. Update Completer to allow for generic Suggestions and Styler.

pub trait Completer {
  type Context;
  fn complete(&mut self, line: &str, pos: usize) -> Vec<Suggestion<Self::Context>>;

  // We *probably* want to instead have it be a separate field on ReedlineMenu::WithCompleter,
  // so that any compatible Styler can be used. But that's maybe something to think on later.
  //
  // This could return one of predefined simple static stylers, or something custom for the completer.
  fn styler<'a>(&'a self) -> &'a dyn Styler<Self::Context>;
}

4. Provide some basic Stylers, eg. one for highlighting the entire item, one highlighting a prefix (for completers with eg. u32 Context). New Stylers could use the base ones for generating regular DisplaySuggestion and then updating just a few fields as needed.

Regarding the Context associated type: it don't necessarily think it's important for performance (though that really depends on a particular completer), but aside from making think a bit (I think) cleaner, it also can serve as a way to declare which Stylers are compatible with which Completers - which in turn would let use decouple the two more, rather than have Completer define a single associated Styler.

terminal_dims: Option<(u16, u16)>

I noticed one of the examples above had this - what would it be used for?

Yea not really sure what that could be used for either, just came into my mind when thinking of things that might be useful to style a suggestion.

I think it would also be useful to store the string the completer used to suggest that suggestion in the Suggestion struct.

I think it would also be useful to store the string the completer used to suggest that suggestion in the Suggestion struct.

@maxomatic458 Would this be used by the Styler trait to have more information for styling?

Not a huge issue I suppose, but makes it a little harder to get things going (and might have some weird implications for non-deterministic/pure matching, maybe if somebody decided to do some server-side completions).

That's a good point, @qfel, running matching twice wouldn't be good for non-deterministic/time-consuming suggestions.

That said, match_indices won't be enough if we ever decide we want more than 2 styles in a single suggestion, e.g. green for the first half, red for the second half, underlines for the matched characters. I personally cannot think of why this would happen, but it is possible we need advanced styling in the future.

@maxomatic458's display_value idea solves that, but I was thinking that instead of a single field, we could instead have two fields, display_value_unselected and display_value_selected. Instead of providing a style, each completer could style its suggestions completely on its own, using helper functions provided by Reedline. The advanced styling that would have gone into the Styler trait would be done by each completer instead, though most would just Reedline's helpers for that. No match_indices field would be needed in each suggestion itself. Thoughts?

Would this be used by the Styler trait to have more information for styling?

@ysthakur that could be also be used outside of styling. Currently we have the helper function "complete_with_base_ranges", that basically does the same thing already, but instead of collecting the actual strings, we just get the ranges in the buffer (i think).

So an extra field that holds the "completion base value" could be used to store that instead.
Currently thats required to align the IDE menu suggestions with the text already typed in the terminal.

Hi,
does this also solves the issue of
mismatch in highlight.
the create_string is assuming the match string start at the front.

Thanks.
Kunam Balaram Reddy

updating create_string method is enough i think.
split suggestion.value into three parts and add highlight to matched part and return a formatted string.

like this

let suggestion_style_prefix = suggestion.clone()
        .style
        .unwrap_or(Style::new().bold().fg(Color::Cyan))
        .prefix();
let extras = suggestion.extra.as_ref().unwrap();
let match_str = extras.get(0).unwrap();
let splits = Self::split_string_by_pattern(&suggestion.value, &match_str).unwrap();

format!(
    "{}{}{}{}{}{}{}{}{}{:>empty$}{}{}",
    suggestion_style_prefix,
    splits.0,
    Style::default().underline().prefix(),
    match_str,
    RESET,
    suggestion_style_prefix,
    splits.2,
    RESET,
    Color::Yellow.normal().prefix(),
    "",
    RESET,
    "\r\n",
    empty = empty_space,
)

for example.

@ysthakur The regex dependency remains in Cargo.lock because it's a transitive dependency of rstest (our test framework)

This PR now does the ANSI sequence parsing itself, rather than using regex. It should be ready to merge now (built and ran Nushell with these changes and things seem fine). Scratch that, need to merge nushell/nushell#15764 first so that the new field added to Suggestion won't cause problems (edit: merged).

I think this is ready to be merged! @maxomatic458 could you look over it to make sure it looks alright?